r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

Can we add a test to test that this works ?

E.g. a build job that tries to cargo miri one of the std_detect examples ?

Can we add a test to test that this works ?

E.g. a build job that tries to cargo miri one of the std_detect examples ?

On our side or on the miri side?

Like this one: https://github.com/rust-lang/stdarch/blob/master/crates/std_detect/tests/cpu-detection.rs

On our side.

I was kind of thinking maybe has_cpuid is where miri should just say false -- but I'll leave that to you, I have not seen this code here before nor did I even know it existed.

On our side.

Here's a snippet for running Miri on C.

On our side.

Here's a snippet for running Miri on C.

If someone volunteers to update the test runner with that would be great :)

I opened an issue for the CI task since I'd like to merge this now, is it fine to ignore the cirrus-ci?

@lu-zero see this: rust-lang/rust#64683 (comment)

We probably should remove miri support, since code that uses is_x86_feature_detected! and wants to run under miri should handle that at a higher level. Maybe just panic!("is_x86_feature_detected! cannot be called under miri.") instead ?

Can't we make it so that the optimization "features known at compile-time statically return true" does not happen in Miri? Where is that optimization even implemented?

This code never has UB in Rust today:

fn main() {
    if !is_x86_feature_detected!("sse") {
       if cfg!(target_feature = "sse") {
          // invoke UB
          // note: this never happens, since SSE cannot be 
          // enabled and disabled at the same time
       }
    } 
}

If is_x86_feature_detected! were to return false under miri, but cfg(target_feature) continues to report the features that are enabled for the target at compile-time, then that example would have UB, at least, when run under miri.

The optimization is implemented here: https://github.com/rust-lang/stdarch/blob/master/crates/std_detect/src/detect/macros.rs#L97

If is_x86_feature_detected! were to return false under miri, but cfg(target_feature) continues to report the features that are enabled for the target at compile-time, then that example would have UB, at least, when run under miri.

Fair. OTOH, UB in Miri is not the total catastrophe it usually is. So that seems better to me than the alternative.

In practice, there is also a lot of Rust code like this:

if is_x86_feature_detected!("avx") {
  do_avx_thingy()
} else {
  do_sse_fallback() // will try to run an intrinsic that miri doesn't support
}

so if we were to always return false, that code will need to be massaged to become like this:

if is_x86_feature_detected!("avx") {
  do_avx_thingy()
} else if is_x86_feature_detected!("sse") {
  do_sse_fallback()
} else {
  do_miri_fallback()
}

and that would work.

With the current implementation, this example will initially error with a deep error in the miri stack when it encounters the first sse intrinsic within do_sse_fallback that miri does not support. If we were to error, this error would surface earlier. To fix these errors, one would need to write:

if cfg!(miri) {
  do_miri_fallback()
} else if is_x86_feature_detected!("avx") {
  do_avx_thingy()
} else {
  do_sse_fallback()
} 

instead.

if is_x86_feature_detected!("avx") {
  do_avx_thingy()
} else {
  do_sse_fallback() // will try to run an intrinsic that miri doesn't support
}

Sure, code that always requires SIMD cannot work in Miri without being adjusted.

if cfg!(miri) {
  do_miri_fallback()
} else if is_x86_feature_detected!("avx") {
  do_avx_thingy()
} else {
  do_sse_fallback()
} 

This would be an alternative, yes. But using feature flags seems more generally applicable and also more portable across platforms? Like, the first code above does not work on ARM.

But using feature flags seems more generally applicable and also more portable across platforms?

The thing is feature flags are currently exclusively additive, while "miri" would be like a feature flag that disables some things, and that often ends up causing trouble.

For example, by disabling all features, miri would not only disable SIMD stuff like sse, but also 64-bit floating point arithmetic, e.g.,

if is_x86_feature_detected!("sse") {
   // 64-bit fp arithmetic
} else {
   // 80-bit fp arithmetic is enabled when SSE is disabled
}

so when running on miri, code that queries which kind of arithmetic is available might end up being subtly confused by the answer of is_x86_feature_detected!.

I don't mind much either way, but we currently promise that is_x86_feature_detected and cfg(target_feature) are consistent with each other, and I am not sure yet of the implications of violating that promise under miri. I suspect that its probably not harmful in any way since, well, the program is running under miri, but I'd prefer to try to find a better solution to this problem.

Like, the first code above does not work on ARM.

is_x86_feature_detected is not available on arm, so if you try to use it you get a compilation error. You need to gate uses of is_x86_feature_detected on a #[cfg(target_arch = "x86"/"x86_64")] to write portable code, but miri would be another thing that all "branches" would need to handle as well to make sure that there is a fallback that does not use anything that miri does not support.

Thinking more about the whole problem, code like this:

if cfg!(target_feature = "sse") {
    sse_fn()
} else {
    miri_fallback()
}

won't work under miri either because independently of what is_x86_feature_detected! does, cfg! returns some different result. I don't think it is reasonable to change that code to use is_x86_feature_detected! because there are valid reasons not to do run-time feature detection.

I think I would prefer for is_x86_target_feature_detected to be consistent with #[cfg(target_feature)], but that would require miri somehow telling rustc to either disable those features when compiling, or to "lie" in some particular way when expanding those.

but that would require miri somehow telling rustc to either disable those features when compiling

That would be useful for alternative codegen backends. (not much anymore for cg_clif though, as most sse usage I have seen is already emulated by it at the moment)

Miri already sets some flags for all code it runs; we could add a -C something there to kill all target features, maybe?

The thing is feature flags are currently exclusively additive, while "miri" would be like a feature flag that disables some things, and that often ends up causing trouble.

miri is not a cargo feature flag though. It's more like debug_assertions or so. I don't think there is any fundamental requirement that --cfg flags have to be additive.

@RalfJung

Miri already sets some flags for all code it runs; we could add a -C something there to kill all target features, maybe?

Yes, maybe. I'm not sure, but I think this could work. The main "issue" is that target features are part of what constitutes a target, but since when running on miri we don't do any kind of codegen, then I think the flag can just tell cfg!(target_feature = "...") to always expand to false.

I don't think there is any fundamental requirement that --cfg flags have to be additive.

Not for --cfg flags, but there is one for target-features at least when used in #[target_feature(enable = "x")] (a disable = alternative does not exist), and when used in is_x86_feature_detected!("x").

@bjorn3

That would be useful for alternative codegen backends.

Maybe? When people use -C target-cpu=native or similar to "detect" target features, currently, rustc asks the backend to do this kind of detection, so the backend already has a certain level of control over this.

The main issue is that target-features are a property of the target, e.g., i686-unknown-linux-gnu and i586-unknown-linux-gnu mostly only differ on their target-features. If the backend does not support the features that the target specifies, then it probably is better for the backend to just error on that. I mean, if the user requests to compile the whole program with some feature, e.g., -soft-float and the backend doesn't support it, then trying to continue compilation silently using +soft-float instead would be like trying to compile code for a 64-bit target on a backend that only supports 32-bit targets and just silently using 32-bit pointers and usize instead while having e.g. the cfgs about target-pointer-width report 64.

I think it is unrealistic to expect that all backends (or miri) support all intrinsics or language features at least from the get go. I think the best thing to do would be to just disable them for those backends. E.g. we could add #[cfg(not(cranelift)] to the std::arch module, and all code that uses it would fail to compile when trying to use those backends. That's obviously too harsh, so we could do it on an intrinsics per intrinsic basis.

For miri, as long as the program doesn't trying to execute them at run-time, there is no issue, so some kind of "run-time" detection is necessary. The problem is that the run-time detection system is coarse, in the sense that detecting "sse" means that all SSE1 is supported, and that's like hundreds of intrinsics. There is no finer grained way right now to just detect whatever intrinsics you need. The current attempt at handling miri inside is_x86_feature_detected! and cfg!(target_feature) is a "best effort" kind of thing to try to support as much code as possible, but for libraries that I would want to actually run under miri and that use intrinsics (e.g. hashbrown), what we actually do there IIRC is just completely #[cfg(not(miri))] all the intrinsics away, so that they don't even attempt to get compiled. Maybe if we succeed here with changing what #[cfg(target_feature)] reports under miri, those cfgs won't be necessary anymore.

Maybe? When people use -C target-cpu=native or similar to "detect" target features, currently, rustc asks the backend to do this kind of detection, so the backend already has a certain level of control over this.

1. The problem is that the default list for a target already contains certain features, which cant be disabled without writing your own target json.
2. I cant find where -Ctarget-cpu affects the target features enabled from the prespective of rustc, only where it affects LLVM.

I think it is unrealistic to expect that all backends (or miri) support all intrinsics or language features at least from the get go. I think the best thing to do would be to just disable them for those backends. E.g. we could add #[cfg(not(cranelift)] to the std::arch module, and all code that uses it would fail to compile when trying to use those backends. That's obviously too harsh, so we could do it on an intrinsics per intrinsic basis.

Most SIMD code does runtime feature detection, so compiling code with those intrinsics should work, as you have to patch the whole world otherwise. Only reaching them during runtime should panic or crash. However because of the enabled target features, the rumtime detection macros return true for some features.

I think making rustc think that no or only some target features are enabled depending on the backend and not on the target specification would be helpful for both miri and alternative codegen backends.

I think making rustc think that no or only some target features are enabled depending on the backend and not on the target specification would be helpful for both miri and alternative codegen backends.

If we were to go down this route, how would we decide, e.g., whether some target-feature should return true for cranelift ? If we were to require that cranelift must support all the intrinsics that the feature enables, then I think it's likely that we might never be able to enable any feature for cranelift. If we decide that cranelift only needs to partially support the feature, then some code will panic at run-time when hitting an unsupported intrinsic.

If we were to go down this route, how would we decide, e.g., whether some target-feature should return true for cranelift ?

We could have a query of CodegenBackend method returning the list of enabled target_features. The former would also work for miri, but the later seems cleaner to me.

If we were to go down this route, how would we decide, e.g., whether some target-feature should return true for cranelift ?

I dont really know, but maybe all used intrinsics by most crates using that feature supported would be a good heuristic.